Skip to content

fix: prevent integer overflow in DiskHealthReport and document pinned Ookla CLI version#442

Merged
laurentiu021 merged 1 commit into
mainfrom
fix/disk-health-overflow-and-ookla-version
May 19, 2026
Merged

fix: prevent integer overflow in DiskHealthReport and document pinned Ookla CLI version#442
laurentiu021 merged 1 commit into
mainfrom
fix/disk-health-overflow-and-ookla-version

Conversation

@laurentiu021

@laurentiu021 laurentiu021 commented May 19, 2026 •
edited by coderabbitai Bot
Loading

Copy link
Copy Markdown
Owner

Summary

Fix two remaining bugs from the full code review (BUG-008, BUG-009).

Changes

DiskHealthReport.cs (BUG-009)

  • Fixed potential integer overflow in HealthPercent when ReadErrors or WriteErrors
    exceed int.MaxValue. The cast (int)ReadErrors.Value * 5 could wrap to a negative
    number, causing the score to increase instead of decrease.
  • Fix: use long arithmetic (ReadErrors.Value * 5L) before clamping with Math.Min(..., 20L).

SpeedTestService.cs (BUG-008)

  • Added maintenance comment documenting the pinned Ookla CLI version (1.2.0) and
    explaining the update procedure. Authenticode signature verification ensures binary
    integrity regardless of version.

Previously fixed (verified, no action needed)

  • BUG-001 (WindowsFeaturesVM CanExecute) — already calls NotifyCanExecuteChanged
  • BUG-002 (DuplicateFileGroup WastedBytes) — already has [NotifyPropertyChangedFor]
  • BUG-003 (TuneUpService HRESULT) — already checks return value
  • BUG-004 (UpdateService pre-release) — already filters in Map()
  • BUG-005 (ServiceManagerService StartPending) — already checks status
  • BUG-006 (DiskAnalyzerService empty dirs) — correctly tracks exceptions
  • BUG-007 (WindowsUpdateVM Shutdown) — already uses ?.Shutdown()
  • SEC-001 (SpeedTestService fake hashes) — no fake hashes exist; uses Authenticode

Testing

  • Build: 0 errors
  • No behavior change for normal values (deduction capped at 20 regardless)

Summary by CodeRabbit

  • Bug Fixes

    • Fixed an integer overflow issue in disk health percentage calculations.

  • Documentation

    • Updated changelog with maintenance notes and fix documentation.

Review Change Stack

@coderabbitai

coderabbitai Bot commented May 19, 2026 •
edited
Loading

Copy link
Copy Markdown

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 9c44e241-7d70-4a96-b6b3-2a06d8f33d26

📥 Commits

Reviewing files that changed from the base of the PR and between 98952ae and 3275976.

📒 Files selected for processing (3)
  • CHANGELOG.md
  • SysManager/SysManager/Models/DiskHealthReport.cs
  • SysManager/SysManager/Services/SpeedTestService.cs
📜 Recent review details
🔇 Additional comments (3)
SysManager/SysManager/Services/SpeedTestService.cs (1)

313-316: LGTM!

SysManager/SysManager/Models/DiskHealthReport.cs (1)

77-78: LGTM!

CHANGELOG.md (1)

17-21: LGTM!

📝 Walkthrough

Walkthrough

This PR applies a targeted integer overflow prevention fix to DiskHealthReport.HealthPercent calculations and adds maintenance documentation to SpeedTestService regarding Ookla CLI versioning. The changelog documents both updates.

Changes

Bug Fix and Maintenance Updates

Layer / File(s) Summary
DiskHealthReport integer overflow prevention
SysManager/SysManager/Models/DiskHealthReport.cs, CHANGELOG.md		 HealthPercent now computes error-count penalties using long arithmetic (* 5L, Math.Min(..., 20L)) before casting to int, eliminating overflow risk during multiplication and clamping. Changelog entry documents the fix.
SpeedTestService Ookla CLI documentation
SysManager/SysManager/Services/SpeedTestService.cs		 EnsureOoklaAsync gains maintenance comments clarifying that the Ookla CLI version (1.2.0) is pinned and that Authenticode signature verification ensures binary integrity independent of the pinned version.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~5 minutes

Possibly related PRs

  • laurentiu021/SystemManager#294: Overlaps with SpeedTestService.EnsureOoklaAsync documentation about pinned Ookla CLI version and security hardening for downloads.
  • laurentiu021/SystemManager#266: DiskHealthReport.HealthPercent change impacts the disk-health input used by HealthScoreService to compute overall dashboard health scores.
  • laurentiu021/SystemManager#403: Changes to SpeedTestService.EnsureOoklaAsync are in the same method that the referenced PR hardens for Ookla execution and download safety.

Poem

🐰 A rabbit hops through overflow's snare,
with long arithmetic cast with care,
while Ookla's CLI stays pinned so tight,
and Authenticode keeps downloads right!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main changes: fixing integer overflow in DiskHealthReport and documenting the pinned Ookla CLI version, which directly aligns with the changeset.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/disk-health-overflow-and-ookla-version

Warning

Review ran into problems

🔥 Problems

Git: Failed to clone repository. Please run the @coderabbitai full review command to re-trigger a full review. If the issue persists, set path_filters to include or exclude specific files.

Comment @coderabbitai help to get the list of available commands and usage tips.

@laurentiu021
laurentiu021 merged commit 21313eb into main May 19, 2026
5 checks passed
@laurentiu021
laurentiu021 deleted the fix/disk-health-overflow-and-ookla-version branch May 19, 2026 14:43
laurentiu021 added a commit that referenced this pull request May 22, 2026
@laurentiu021
fix: prevent integer overflow in DiskHealthReport and document pinned…
39a374d 
… Ookla CLI version (#442)

Co-authored-by: laurentiu021 <laurentiu021@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@laurentiu021